crypto: add DecompressPubkey, VerifySignature#15615
Conversation
The extra output confuses tools like benchstat.
| // VerifySignature checks that the given pubkey created signature over message. | ||
| // The signature should be in [R || S] format. | ||
| func VerifySignature(pubkey, msg, signature []byte) bool { | ||
| if len(msg) != 32 || len(signature) != 64 || len(pubkey) == 0 { |
There was a problem hiding this comment.
Shouldn't len(pubkey) be one of two values ? So either compressed or uncompressed at 65 bytes, or compressed at 33 bytes. If so, I'd prefer to check against one of those. Better to be strict whenever possible.
There was a problem hiding this comment.
I'd prefer to leave it to libsecp256k1 to check for the length. If you disagree strongly we can add the check here too. The check for zero is needed to prevent a crash for the &pubkey[0] construction.
| return secp256k1.RecoverPubkey(hash, sig) | ||
| } | ||
|
|
||
| // SigToPub returns the public key that created the given signature. |
There was a problem hiding this comment.
"SigToPub returns the public key" -> "SigToPub returns the uncompressed public key"
There was a problem hiding this comment.
Compressed/uncompressed doesn't matter for SigToPub because the return value isn't encoded.
| "github.com/ethereum/go-ethereum/crypto/secp256k1" | ||
| ) | ||
|
|
||
| // Ecrecover returns the public key that created the given signature. |
There was a problem hiding this comment.
"Ecrecover returns the public" -> "Ecrecover returns the uncompressed public"
| // DecompressPubkey parses a public key in the 33-byte compressed format. | ||
| func DecompressPubkey(pubkey []byte) (*ecdsa.PublicKey, error) { | ||
| x, y := secp256k1.DecompressPubkey(pubkey) | ||
| if x == nil { |
There was a problem hiding this comment.
Design question: Why does the underlying method return nil, and we create an error here, instead of returning error from the lower layer?
There was a problem hiding this comment.
I decided this way to simplify the interface. There is only one possible error anyway: "invalid". elliptic.Unmarshal uses the same convention.
There was a problem hiding this comment.
You are right though, maybe this should be changed.
| sig := &btcec.Signature{R: new(big.Int).SetBytes(signature[:32]), S: new(big.Int).SetBytes(signature[32:])} | ||
| key, err := btcec.ParsePubKey(pubkey, btcec.S256()) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Another design question: we're throwing away error-info here, is that really needed?
There was a problem hiding this comment.
btcec is the fallback for platforms that can't link C code. It provides better errors because it's Go code. There is no good way to get those errors from the C version. IMHO it doesn't matter much: the signature is invalid if any of the inputs are.
There was a problem hiding this comment.
Maybe more context: I added this because I want a function that does decompression and verification in one call without any pubkey/signature decoding. We get the compressed pubkey in ENR and need to verify that it created the signature. The fastest and simplest way is to just pass these values into libsecp256k1.
We could go ahead and make it so secp256k1_ecdsa_verify_enc returns different ints for 'invalid signature encoding', 'invalid pubkey encoding' and 'signature mismatch', but it'll just make the code more complicated. Callers won't care: all they want to know is whether the signature checks out.
| } | ||
| if VerifySignature(testpubkey, testmsg, nil) { | ||
| t.Errorf("nil signature valid") | ||
| } |
There was a problem hiding this comment.
More potential testcases:
- VerifySignature(testpubkey, testmsg, sig+extra_bytes)
- VerifySignature(testpubkey, testmsg, testsig[:len(testsig)-2])
| } | ||
|
|
||
| func TestDecompressPubkey(t *testing.T) { | ||
| key, err := DecompressPubkey(testpubkeyc) |
There was a problem hiding this comment.
More potential testcases:
- DecompressPubkey with
nil, slice with < 32 bytes, slice with > 32 bytes
|
@holiman PTAL |
3 participants
We need those operations for ENR.